Issue #1380: Validate python parameters passed to SP compute method #1385
Conversation
src/nupic/bindings/algorithms.i
Outdated
| NTA_THROW << "Invalid data type given for active array." | ||
| << " Expecting 'uint" << sizeof(nupic::UInt) * 8 << "'" | ||
| << " but got '" << PyArray_DTYPE(y)->type << "'"; | ||
| } |
There was a problem hiding this comment.
It might be worth creating a CheckedNumpyVectorWeakRefT by copy-pasting the NumpyVectorWeakRefT in https://github.com/numenta/nupic.core/blob/91b45451ec77aabbf71ee9f3645ec704532664f1/src/nupic/py_support/NumpyVector.hpp#L411 and changing it to do checks in release builds. Then we can reuse it for any dtype. The existing code would check that the dtype is correct (which, strictly speaking, this code doesn't currently do). I like that you throw useful error text -- we'd want that in the Checked class.
This we could change this code to
nupic::CheckedNumpyVectorWeakRefT<nupic::UInt32> inputArray(py_inputArray);
nupic::CheckedNumpyVectorWeakRefT<nupic::UInt32> activeArray(py_activeArray);
self->compute(inputArray.begin(), learn, activeArray.begin());There was a problem hiding this comment.
@mrcslws regarding
The existing code would check that the dtype is correct (which, strictly speaking, this code doesn't currently do)
Can you give an example where this code does not check the dtype of the numpy array passed to the function? I would like to cover that condition with a test.
There was a problem hiding this comment.
@mrcslws Why create the new CheckedNumpyVectorWeakRefT class?
NumpyVectorWeakRefT seems to do what I need.
Any reason why this class should only throw an exception in debug mode?
There was a problem hiding this comment.
I intentionally used NTA_ASSERT in the NumpyVectorWeakRefT. In the SparseMatrix, the parameters are checked in the Python bindings. It would be wasteful to recheck them. The main purpose of the class isn't to check its parameters, it's to expose the array data.
| vector<UInt> out2(nColumns, 0); | ||
| sp.compute(input.data(), false, out1.data()); | ||
| sp.compute(input.data(), false, out2.data()); | ||
| EXPECT_EQ(out1, out2); |
There was a problem hiding this comment.
I don't think this test validates the issue. For 2 reasons:
- your commit eba72d0 (this test) passed OK even without the suggested fix
- the fix is in python related
.ifile. The issue exhibits even in pure c++ - please see Fix SP unstable output, synapse decay #1382 for discussion and example test
that crashes correctly
|
Re-reading the PR, I think the py fix is fine 👍 , but the test should be removed (checks something else and does not reproduce it), instead, make test that validates this fix (python compute called with numpy array where dtype does not match UInt) |
|
@lscheinkman @mrcslws This is clear to merge. I have warned the community of potential problems. |
Fixes #1380
The
swiginterface now validates thenumpy.arraytype making sure it matches the C++ implementation (nupic::UInt). It will now throw an exception when thecomputemethod is called with the wrong data type.